Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix speedscope time interval #434

Merged
merged 2 commits into from
Aug 11, 2021
Merged

Fix speedscope time interval #434

merged 2 commits into from
Aug 11, 2021

Conversation

gjoseph92
Copy link
Contributor

This fixes two things:

  1. With multiple threads/processes, each may be a different length, so the end_value needs to be set independently. (Also before, the end_value was mistakenly the number of threads, not the number of samples in a thread.)
  2. weights are (apparently) not unitless, and need to be given in seconds.

Closes #430

Didn't have time to add tests, but I could if you'd like.

FWIW I was manually testing with:

import threading
import time

threads = [threading.Thread(target=time.sleep, args=(i / 2,)) for i in range(4)]
for t in threads:
    t.start()

for t in threads:
    t.join()

@benfred
Copy link
Owner

benfred commented Aug 11, 2021

looks good - thanks! I tested myself before/after and the output makes a bunch more sense now

(btw - keep up the good work on dask! I use dask for work and love it =).

@benfred benfred merged commit 39ffe83 into benfred:master Aug 11, 2021
@gjoseph92
Copy link
Contributor Author

Keep up the good work on py-spy! It's been invaluable lately. We know you're busy but we'd love to nerd-snipe you on dask/distributed#4825 and dask/distributed#4987 someday :)

manuels pushed a commit to manuels/py-spy that referenced this pull request Aug 16, 2021
@gjoseph92 @manuels
Per-thread env_value & scaled weights for speedscope (benfred#434)
ef8bd54
@gjoseph92
Copy link
Contributor Author

Hey @benfred whenever you have the time, it would help us a ton if you could cut a new py-spy release with this change. I'm profiling multithreaded things for dask, and having the nonsensical times is a little annoying.

@benfred
Copy link
Owner

benfred commented Sep 9, 2021

@gjoseph92 I've tagged a new release - which will be pushed out to pypi/cargo etc with by this CI run https://github.com/benfred/py-spy/actions/runs/1216029122 when it finishes

@gjoseph92
Copy link
Contributor Author

Thank you!!

gjoseph92 added a commit to gjoseph92/dask-pyspy that referenced this pull request Sep 9, 2021
@gjoseph92
Require new py-spy
826ed2b 
benfred/py-spy#434 is very valuable for dask profiles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Time scale off when exporting to speedscope
2 participants
@gjoseph92 @benfred